Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.58%. Comparing base (f1ec415) to head (e97c2e0).
Report is 7 commits behind head on main.

@@           Coverage Diff           @@
##             main    #3443   +/-   ##
=======================================
  Coverage   95.58%   95.58%           
=======================================
  Files         155      155           
  Lines        9357     9357           
  Branches     2746     2784   +38     
=======================================
  Hits         8944     8944           
  Misses        413      413           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hi @m-shaka

So, I've tried replacing intersections to union and as a result, this has decreased type instantiation drastically, that is, by about 70%.

Awesome!

However, union here is not intuitive and this may be a breaking change for users manipulating type of Hono somehow. I'm not certain how they do that, though

I've been thinking about this for a while: We can care less about changing the type definitions of handlers. A few users customize them, but we don't have to care about them though we have to take care of actual runtime behaviors. These are at risk own. So, it's okay to go with this.

Plus, regarding the types, we must care about the performance, most importantly.

Shall we go ahead with this PR?

Found that replacing intersections with unions will break chaning like

      new Hono()
        .get('/bar/:id', c => c.text('foo'))
        .post(c => {
          const id = c.req.param('id') // Woops! `string | undefined`
          return c.text(id)
        })

It's likely there're pitfalls we don't notice so far 🤔

Instead, I'm wondering if we can change the intersection in route method method into union. This will make difference in many projects

apps added through route are independent each other, I suppose

I replaced intersection in route method with union.

I prepared new another script to generate a suitable test case (it's not commited)

import { writeFile } from 'node:fs'
import * as path from 'node:path'

const count = 10
const countSubRoutes = 20

const generateRoutes = (count: number) => {
  let routes = `import { Hono } from '../../../src'
export const app = new Hono()`
  for (let i = 1; i <= count; i++) {
    routes += `
  .route('', new Hono()`
    // each sub app has 20 routes
    for (let j = 1; j <= countSubRoutes; j++) {
        routes += `
    .get('/route${i}-${j}/:id', (c) => {
        return c.json({ ok: true })
    })`
    }
    routes += `
  )`
  }
  return routes
}

const routes = generateRoutes(count)

writeFile(path.join(import.meta.dirname, '../generated/app.ts'), routes, (err) => {
  if (err) { throw err }
  console.log(`${count} routes have been written to app.ts`)
})

Put this into perf-measures/type-check/scripts. Then, you can get a comparison as well

                   Current    Previous
 ---               ---        ---
 Files             248        248
 Lines             113666     113666
 Identifiers       111407     111404
 Symbols           260996     250558
 Types             197294     192000
 Instantiations    1506985    3691507

Instantiaions is down 60%

@m-shaka

I replaced intersection in route method with union.

This is awesome! I also tried it; I could feel it became faster with app.route(). Let's introduce this.

I prepared new another script to generate a suitable test case (it's not committed)

Regarding measuring performance, we can measure two patterns:

• Handlers without app.route()
• Hanlders with app.route()

We can choose only the latter to measure it, but in my opinion, it's better to know both performances because there are cases if a user uses app.route() or not. What do you think of this?

If so, the script and CI process will be a bit complicated. But, for measuring app.route(), you can only add this script with using the current app.ts generated:

const count = 5

const generateRoutes = (count: number) => {
  let routes = `import { Hono } from '../../../src'
import { app as subApp } from './app'
export const app = new Hono()`
  for (let i = 1; i <= count; i++) {
    routes += `
  .route('/${i}', subApp)`
  }
  return routes
}

const routes = generateRoutes(count)
// ...

And create two files:

• generated/app.ts
• generated/app-with-route.ts

Then measure this tsc for each file. Or there may be any good ideas.

@yusukebe Apologies for my long absence.

Regarding the script for the performance measure, how about doing it in another PR?
I like your idea, but it seems like we need to think about how the display of the result should look like.
We'll have two patterns of perf-measure, so the simple table view may not be suitable

you can only add this script with using the current app.ts generated

#3491 is amazing! After merging it and this PR, I'll improve the display of the result

@m-shaka

Regarding the script for the performance measure, how about doing it in another PR?

Agree!

I like your idea, but it seems like we need to think about how the display of the result should look like.
We'll have two patterns of perf-measure, so the simple table view may not be suitable

I'm also thinking about this.

After merging it and this PR, I'll improve the display of the result

Yes!

It looks good for me. Can I merge it?

Go!

@m-shaka

Okay! I just released v4.6.4, so let's release it in v4.6.5.